Repository navigation
perf(terminal): coalesce PTY output into one xterm.write per animation frame (v2 transport) - #5255
Conversation
Agent CLIs (Claude Code especially) emit full-screen repaints as many small PTY chunks. The v2 WebSocket transport wrote each chunk to xterm individually, triggering a parse/render cycle per chunk - the renderer falls behind during streaming output and scrolling turns janky. Batch incoming binary frames in a requestAnimationFrame-aligned coalescer so xterm processes one write per displayed frame. Control messages and socket close flush synchronously first, so exit notices and error lines stay ordered with PTY output. A 1MB pending cap bounds memory when rAF stalls (hidden window). Addresses ParadevAI#2241 for the v2 transport path (the v1 IPC path already has 16ms batching in the main process via DataBatcher; its renderer-side coalescing can follow the same pattern).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces PTY output write coalescing to batch terminal writes and preserve output ordering. A new ChangesPTY Output Write Coalescing
Sequence Diagram(s)sequenceDiagram
participant WebSocket
participant Transport
participant WriteCoalescer
participant Terminal
WebSocket->>Transport: message (binary PTY frame 1)
Transport->>WriteCoalescer: push(ArrayBuffer)
WebSocket->>Transport: message (binary PTY frame 2)
Transport->>WriteCoalescer: push(ArrayBuffer)
WebSocket->>Transport: message (JSON control: title/exit)
Transport->>WriteCoalescer: flushSync()
WriteCoalescer->>Terminal: write(coalesced bytes)
Transport->>Terminal: writeln(control message)
WebSocket->>Transport: close event
Transport->>WriteCoalescer: flushSync()
WriteCoalescer->>Terminal: write(remaining bytes)
Transport->>Transport: mark disconnected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit 3e0be04 on Jun 12, 2026 11:08pm UTC. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
Architecture diagram
sequenceDiagram
participant WS as WebSocket
participant Transport as Terminal WS Transport
participant Coalescer as Write Coalescer
participant XTerm as XTerm Terminal
Note over WS,XTerm: PTY Output Flow (v2 WebSocket Transport)
loop Per WebSocket Message
WS->>Transport: binary frame (PTY chunk)
Transport->>Coalescer: push(chunk)
Coalescer->>Coalescer: buffer chunk, track pendingBytes
alt pendingBytes > 1MB cap
Coalescer->>Coalescer: flushSync()
Coalescer->>XTerm: write(combined buffer)
else rAF not scheduled
Coalescer->>Coalescer: requestAnimationFrame()
end
end
Note over Coalescer: requestAnimationFrame fires
Coalescer->>Coalescer: flushSync() (combine all pending)
Coalescer->>XTerm: write(combined buffer)
Note over WS,XTerm: Control Message Ordering
WS->>Transport: control message (exit, error)
Transport->>Coalescer: flushSync()
Coalescer-->>XTerm: pending PTY bytes written
Transport->>XTerm: writeln("[terminal] exited...")
Note over WS,XTerm: Disconnect / Dispose
WS->>WS: socket close event
Transport->>Coalescer: flushSync()
Coalescer-->>XTerm: remaining bytes written
Transport->>Transport: cleanup socket
Transport->>Transport: disconnect() or disposeTransport()
Transport->>Coalescer: dispose()
Coalescer->>Coalescer: flushSync() (final write)
Coalescer->>Coalescer: mark disposed (reject future pushes)
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge. The coalescer is a well-scoped addition: it only affects how bytes reach xterm, all teardown paths flush correctly, and the behaviour is thoroughly exercised by both unit and transport-level tests. The core WriteCoalescer logic and integration are correct across rAF lifecycle, byte-cap flush, dispose idempotency, and reconnect paths. Two small quality gaps exist: the transport test cancelAnimationFrame mock clears all callbacks by resetting the array instead of deleting by ID, and flushSync() is called unconditionally before every JSON control message rather than only the branches that write to the terminal. The cancelAnimationFrame mock in terminal-ws-transport.test.ts and the unconditional flushSync() before all JSON control messages in terminal-ws-transport.ts are worth a second look, but neither affects production correctness.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/write-coalescer.ts | New module: buffers Uint8Array chunks, drains into a single write() per rAF, with synchronous flush-and-cancel on flushSync(), a 1MB byte cap, and idempotent dispose(). No issues found. |
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | Wires WriteCoalescer into the transport correctly across connect/disconnect/reconnect/liveness-watchdog paths. Minor: flushSync() is called before non-terminal-writing messages, causing unnecessary early flushes. |
| apps/desktop/src/renderer/lib/terminal/write-coalescer.test.ts | 7 unit tests covering coalescing, re-scheduling, flushSync semantics, empty-flush no-op, byte-cap trigger, dispose semantics, and byte-order preservation. Uses correct Map-based rAF mock. |
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.ts | Adds 3 transport-level coalescing tests. Tests are correct but the cancelAnimationFrame mock clears all callbacks rather than only the one matching the given ID. |
Sequence Diagram
sequenceDiagram
participant WS as WebSocket
participant TWT as terminal-ws-transport
participant WC as WriteCoalescer
participant RAF as requestAnimationFrame
participant XTerm as xterm.js
WS->>TWT: binary frame (PTY bytes)
TWT->>WC: push(Uint8Array)
WC->>RAF: schedule frame (if not already scheduled)
Note over WC: pending chunks accumulate
WS->>TWT: binary frame (PTY bytes)
TWT->>WC: push(Uint8Array)
Note over WC: appended to pending
RAF->>WC: frame callback fires
WC->>WC: cancel frameId, concat chunks
WC->>XTerm: write(combined Uint8Array)
alt JSON control message arrives
WS->>TWT: JSON frame (exit/title/attached/...)
TWT->>WC: flushSync() [cancel rAF, write pending immediately]
WC->>XTerm: write(pending bytes)
TWT->>XTerm: writeln(exit notice) / other control action
end
alt Socket closes
WS->>TWT: close event
TWT->>WC: flushSync()
WC->>XTerm: write(pending bytes)
end
alt disconnect() / disposeTransport() / reconnect via connect()
TWT->>WC: "dispose() -> flushSync() + mark disposed"
WC->>XTerm: write(any remaining bytes)
Note over WC: future push() calls silently dropped
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.ts:143-145
**`cancelAnimationFrame` mock ignores the ID and clears all callbacks**
The mock resets the entire `frameCallbacks` array on every `cancelAnimationFrame` call regardless of which frame ID was passed. The real API only cancels the one frame identified by the given ID. The coalescer enforces a single-pending-rAF invariant today, so no current test is wrong. But if a future test exercises a scenario where two callbacks happen to be queued simultaneously, the mock would incorrectly cancel the unrelated callback and the test would silently produce a false pass. The write-coalescer unit tests already use the correct `Map`-based mock — using the same pattern here avoids the fragility.
### Issue 2 of 2
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:439-441
**`flushSync` called unconditionally before all JSON messages, including non-writing ones**
`flushSync()` is called for every JSON control message — including `attached` and `title`, which never write to the terminal. The ordering guarantee is only actually needed before messages that call `terminal.writeln` (the `exit` branch and the JSON-parse-failure path). PTY chunks arriving in the same event-loop turn as an `attached` acknowledgment (e.g., end of a replay burst) get flushed a frame early, defeating coalescing for that batch. Deferring the guard to only the branches that write to the terminal would preserve ordering where it matters while allowing fully-coalesced delivery for the `title`/`attached`/`error` branches.
Reviews (1): Last reviewed commit: "perf(terminal): coalesce PTY output into..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
cancelAnimationFrame mock ignores the ID and clears all callbacks
The mock resets the entire frameCallbacks array on every cancelAnimationFrame call regardless of which frame ID was passed. The real API only cancels the one frame identified by the given ID. The coalescer enforces a single-pending-rAF invariant today, so no current test is wrong. But if a future test exercises a scenario where two callbacks happen to be queued simultaneously, the mock would incorrectly cancel the unrelated callback and the test would silently produce a false pass. The write-coalescer unit tests already use the correct Map-based mock — using the same pattern here avoids the fragility.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.ts
Line: 143-145
Comment:
**`cancelAnimationFrame` mock ignores the ID and clears all callbacks**
The mock resets the entire `frameCallbacks` array on every `cancelAnimationFrame` call regardless of which frame ID was passed. The real API only cancels the one frame identified by the given ID. The coalescer enforces a single-pending-rAF invariant today, so no current test is wrong. But if a future test exercises a scenario where two callbacks happen to be queued simultaneously, the mock would incorrectly cancel the unrelated callback and the test would silently produce a false pass. The write-coalescer unit tests already use the correct `Map`-based mock — using the same pattern here avoids the fragility.
How can I resolve this? If you propose a fix, please make it concise.| // Control messages may write to the terminal (exit notice, invalid | ||
| // payload); flush queued PTY bytes first so output stays ordered. | ||
| transport._writeCoalescer?.flushSync(); |
There was a problem hiding this comment.
flushSync called unconditionally before all JSON messages, including non-writing ones
flushSync() is called for every JSON control message — including attached and title, which never write to the terminal. The ordering guarantee is only actually needed before messages that call terminal.writeln (the exit branch and the JSON-parse-failure path). PTY chunks arriving in the same event-loop turn as an attached acknowledgment (e.g., end of a replay burst) get flushed a frame early, defeating coalescing for that batch. Deferring the guard to only the branches that write to the terminal would preserve ordering where it matters while allowing fully-coalesced delivery for the title/attached/error branches.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Line: 439-441
Comment:
**`flushSync` called unconditionally before all JSON messages, including non-writing ones**
`flushSync()` is called for every JSON control message — including `attached` and `title`, which never write to the terminal. The ordering guarantee is only actually needed before messages that call `terminal.writeln` (the `exit` branch and the JSON-parse-failure path). PTY chunks arriving in the same event-loop turn as an `attached` acknowledgment (e.g., end of a replay burst) get flushed a frame early, defeating coalescing for that batch. Deferring the guard to only the branches that write to the terminal would preserve ordering where it matters while allowing fully-coalesced delivery for the `title`/`attached`/`error` branches.
How can I resolve this? If you propose a fix, please make it concise.
|
Thank you so much @thebrownproject ! |
Problem
Addresses #2241 / #2244 for the v2 WebSocket terminal path.
Agent CLIs — Claude Code especially — emit full-screen repaints as many small PTY chunks. The v2 transport currently calls
terminal.write()once per WebSocket frame with no batching anywhere in the pipeline (unlike the v1 IPC path, which at least has 16ms batching in the main process viaDataBatcher). Each chunk triggers an xterm parse/render cycle, so during streaming output the renderer falls behind and scrolling turns visibly janky.Real-world report: running Claude Code inside the Superset 2 beta, scrolling is laggy while the same session in a native GPU terminal (Warp) is smooth. Markdown/code views in Superset scroll fine — only the embedded terminal struggles, and only under streaming TUI output.
Fix
Adds a small
write-coalescermodule and wires it intoterminal-ws-transport:write()perrequestAnimationFrame, aligning parse/render cost with the display refresh rate regardless of chunk count — the fix suggested in [perf] Terminal stream events lack client-side batching before xterm.write() #2241, applied to the v2 path.connect()and disposed (with a final flush) ondisconnect/disposeTransport.Tests
bun test src/renderer/lib/terminal/: 234 pass / 1 fail — the failure ("recovers a half-open socket after the machine resumes from sleep") also fails on an untouched checkout ofmainin my environment, so it appears unrelated.Notes
useTerminalStream) renderer-side coalescing is a natural follow-up using the same module; left out to keep this reviewable.suggestedRendererTypeWebGL fallback (one context loss permanently downgrades all terminals to the DOM renderer until restart) — happy to look at that separately if useful.Summary by cubic
Coalesces PTY output in the v2 WebSocket terminal path into one
xterm.write()per animation frame to reduce parse/render churn and smooth scrolling. Addresses #2241 / #2244 by aligning terminal work with display refresh.write-coalescerthat batches incoming binary PTY frames and writes once perrequestAnimationFrame.connect()and disposed ondisconnect/disposeTransport.Written for commit 895bb2e. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Tests